Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cluster): Extend the available attributes for the proxmox_virtual_environment_cluster_options resource #1241

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

svengreb
Copy link
Contributor

@svengreb svengreb commented Apr 27, 2024

Contributor's Note

  • I have added / updated documentation in /docs for any user-facing features or additions.
  • I have added / updated acceptance tests in /fwprovider/tests for any new or updated resources / data sources.
  • I have ran make example to verify that the change works as expected.

Summary

This pull request implements the next-id and notify PVE API cluster options.

The next-id attribute allows to control the range for the next free VM ID. It is implemented as object and can be used in the proxmox_virtual_environment_cluster_options resource like this:

resource "proxmox_virtual_environment_cluster_options" "options" {
  next_id = {
    lower = 200
    upper = 299
  }
}

Note that the minimum and maximum values are unfortunately not documented in the PVE API explorer but can be found in the web UI where the form fields have validations!

The notify PVE API attribute is also an object that has all the PVE API fields:

resource "proxmox_virtual_environment_cluster_options" "options" {
  notify = {
    ha_fencing_mode            = "never"
    ha_fencing_target          = "default-matcher"
    package_updates            = "always"
    package_updates_target     = "default-matcher"
    package_replication        = "always"
    package_replication_target = "default-matcher"
  }
}

Note that the "fencing" attribute names have been adjusted to better reflect their meaning since they are scoped to the Proxmox VE HA fencing feature. All attributes with the _target suffix are names for the Proxmox VE notifications matchers.

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

@svengreb svengreb changed the title feat(cluster): Extend the available attrbites for the proxmox_virtual_environment_cluster_options resource feat(cluster): Extend the available attributes for the proxmox_virtual_environment_cluster_options resource Apr 27, 2024
@svengreb svengreb force-pushed the feature/cluster-options-extend branch 2 times, most recently from 167e5ec to 908b9a7 Compare April 27, 2024 10:13
@bpg
Copy link
Owner

bpg commented Apr 27, 2024

Thanks for the PR! 👍🏼 I'll review it over the weekend.

Sorry about the failed Qodana checks, I'm still experimenting with different static code analysis tools.

Note that the minimum and maximum values are unfortunately not documented in the PVE API explorer but can be found in the web UI where the form fields have validations!

Just fyi, it is documented in https://pve.proxmox.com/pve-docs/api-viewer/#/nodes/{node}/qemu (POST), and also in many references in the admin guide https://pve.proxmox.com/pve-docs/pve-admin-guide.html#_strong_qm_strong_qemu_kvm_virtual_machine_manager

@bpg
Copy link
Owner

bpg commented Apr 27, 2024

@all-contributors please add @svengreb for code

Copy link
Contributor

@bpg

I've put up a pull request to add @svengreb! 🎉

@svengreb
Copy link
Contributor Author

svengreb commented Apr 28, 2024

Sure, take your time 😃

Just fyi, it is documented in https://pve.proxmox.com/pve-docs/api-viewer/#/nodes/{node}/qemu (POST), and also in many references in the admin guide https://pve.proxmox.com/pve-docs/pve-admin-guide.html#_strong_qm_strong_qemu_kvm_virtual_machine_manager

Oh, I haven't checked other endpoints, but it makes sense that these numbers must be documented somewhere related to VMs itself!
I'll update the notes I added to the code regarding these numbers.

This commit implements the `next-id` and `notify` PVE API cluster
options.

The `next-id` attribute allows to control the range for the next free
VM ID. It is implemented as object and can be used in the
`proxmox_virtual_environment_cluster_options` resource and can be used
like this:

```terraform
resource "proxmox_virtual_environment_cluster_options" "options" {
  next_id = {
    lower = 200
    upper = 299
  }
}
```

Note that the minimum and maximum values are unfortunately not
documented in the PVE API explorer but can be found in the web UI where
the form fields have validations!

The `notify` PVE API attribute is also an object that has all the PVE
API fields:

```terraform
resource "proxmox_virtual_environment_cluster_options" "options" {
  notify = {
    ha_fencing_mode            = "never"
    ha_fencing_target          = "default-matcher"
    package_updates            = "always"
    package_updates_target     = "default-matcher"
    package_replication        = "always"
    package_replication_target = "default-matcher"
  }
}
```terraform

Note that the "fencing" attribute names have been adjusted to better
reflect their meaning since they are scoped to the Proxmox VE HA fencing
feature [1]. All attributes with the `_target` suffix are names for the
Proxmox VE notifications matchers [2].

[1]: https://pve.proxmox.com/wiki/Fencing
[2]: https://pve.proxmox.com/pve-docs/chapter-notifications.html#notification_matchers

Signed-off-by: Sven Greb <[email protected]>
@svengreb svengreb force-pushed the feature/cluster-options-extend branch from 908b9a7 to 6bd10c0 Compare April 28, 2024 07:22
Comment on lines +596 to +600
Validators: []validator.String{
stringvalidator.UTF8LengthAtLeast(1),
stringvalidator.RegexMatches(regexp.MustCompile(`^\S|^$`), "must not start with whitespace"),
stringvalidator.RegexMatches(regexp.MustCompile(`\S$|^$`), "must not end with whitespace"),
},
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validation is duplicated few times in the schema, so could extracted in a shared validator.

Copy link
Contributor Author

@svengreb svengreb Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, it's basically a validator that could be applied to (almost) all strings, even when the Proxmox VE API does not check or enforce it.

Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for adding this feature!

There are few minor comments that we can address later. I'll probably incorporate them into my ongoing work on the VM resource.

LGTM! 🚀

@@ -285,6 +403,24 @@ func (m *clusterOptionsModel) importFromOptionsAPI(
m.MigrationNetwork = types.StringNull()
}

if opts.NextID != nil {
m.NextID = &clusterOptionsNextIDModel{}
lower := int64(*opts.NextID.Lower)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can add something like .PointerInt64() to CustomInt64 type to simplify this conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I also thought about it, but decided to solve it this way to get the implementation done.

@bpg bpg merged commit 2eb36f4 into bpg:main Apr 30, 2024
7 checks passed
@svengreb svengreb deleted the feature/cluster-options-extend branch April 30, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants